Skip to content

Improve handling of read errors during sketch enumeration #1438

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

Improve handling of read errors during sketch enumeration #1438

wants to merge 4 commits into from

Conversation

matthijskooijman
Copy link
Collaborator

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • What kind of change does this PR introduce?

It improves handling of errors while enumerating the files in a sketch when loading a sketch. It is a replacement for #1201.

The original usecase for this PR is adding a compile_commands.json symlink to the sketch root directory. My autocomplete setup (or any that uses clangd with default settings) needs compilation_commands.json in the root directory to allow autodiscovering compilation options. Since arduino-cli generates it in the build directory, I create a symlink to the file in the build directory. However, when the build directory is cleaned (i.e. after a reboot), this results in a broken symlink until the file is regenerated. Currently, this broken symlinks prevents loading the sketch, which prevents creating a new compile_commands.json file.

  • What is the current behavior?
  1. When any file or directory in the sketch directory (including hidden files, files in hidden directories and files with unsupported extensions) fail a stat call, loading the sketch fails. This includes broken symlinks and (I think) permission errors on directories that prevent stat'ing files in the dir (untested).
  2. When any file in the actual sketch filelist (so excluding hidden files, files in hidden directories and files with unsupported extensions) fail an open call, they are removed from the sketch filelist silently.
  • What is the new behavior?
  1. Some stat errors (DoesNotExist and ErrPermission) no longer cause failure, preventing sketch load failures for files that are not actually needed.
  2. All open failures now cause a failure, preventing silently ignoring files that would normally be part of the sketch.
  3. compile_commands.json is now excluded from the sketch filelist, to allow users to make a symlink in the sketch root that points to this file in the build directory, without including the file in the sketch and without breaking when the symlink is broken (because the file needs to be generated (again)).

This might cause sketches that used to work to now break, though probably only sketches that have some weirdness (broken symlinks, permission problems, etc.) Also, part of the changes here revert or revise parts of #1353, so some of those breaking sketches were probably already broken before #1353.

  • Other information:

This PR relates to:

I'm not quite convinced that the implementation in this PR is ideal (hence it is a draft). It could probably be improved if we implement the suggestion in #1353 (comment). So, I would welcome feedback about the behavior that this PR introduces, more than the actual implementation of it.

The first commit in this PR introduces a testcase for sketches with (working or broken) symlinks, but the "working symlink" case seems to fail to compile on Windows somehow (I tested just the first commit, which seems to create the symlink successfully given the lack of an exception, but then produces a link error that suggests the symlink is not included in the build). Maybe the symlink is created but the subsequent opening of it fails, which causes it to be silently ignored? If so, then a testrun of the full PR should show an error. We'll know once I submit this PR and the integration tests complete :-)

@matthijskooijman matthijskooijman mentioned this pull request Sep 15, 2021
Closed
5 tasks
@matthijskooijman
Copy link
Collaborator Author

The first commit in this PR introduces a testcase for sketches with (working or broken) symlinks, but the "working symlink" case seems to fail to compile on Windows somehow (I tested just the first commit, which seems to create the symlink successfully given the lack of an exception, but then produces a link error that suggests the symlink is not included in the build). Maybe the symlink is created but the subsequent opening of it fails, which causes it to be silently ignored? If so, then a testrun of the full PR should show an error. We'll know once I submit this PR and the integration tests complete :-)

After fixing two more small issues with the testcase, the github test run reveals the problem:

E + Error during build: Can't open sketch: open C:\Users\runneradmin\AppData\Local\Temp\tmp3uppf06d\CompileIntegrationTestSymlinkOk\link.ino: The process cannot access the file because it is being used by another process.

It turns out that the NamedTemporyFile I used to create a target for the working symlink necessarily keeps the file opened (since it will be deleted on close), but on Windows that prevents opening it a second time (due to the O_TEMPORARY flag used). This is considered a bug in Python, but as of yet still unfixed.

I've pushed another commit that should fix the testcase, by passing delete=False to NamedTemporaryFile, which allows closing the file after writing to it. To ensure it is still deleted, I added a try/finally block to delete the file.

@silvanocerza
Copy link
Contributor

Integration tests are failing as of now because the CLI fails to install adafruit:samd tries to install an ARM tool from an URL with an bad certificate. Don't think we can do much about it other than wait.

$ arduino-cli core install adafruit:[email protected]   
Downloading packages...
Error during install: Error downloading tool adafruit:arm-none-eabi-gcc@9-2019q4: Get "https://developer.arm.com/-/media/Files/downloads/gnu-rm/9-2019q4/gcc-arm-none-eabi-9-2019-q4-major-x86_64-linux.tar.bz2": x509: certificate signed by unknown authority
``

@silvanocerza
Copy link
Contributor

Am not sure the arduino-cli should handle this case. My main concern is the fact that the issue you're having could be solved in a different way by calling compile with the --export-binaries flag so that binaries are exported build/<FQBN> and let clangd read it from there.
I wouldn't make this big of change in the CLI to solve an issue that only requires some more configuration. 😕

@matthijskooijman
Copy link
Collaborator Author

Integration tests are failing as of now because the CLI fails to install adafruit:samd tries to install an ARM tool from an URL with an bad certificate. Don't think we can do much about it other than wait.

Heh. In any case, I think I did see the Windows integration tests pass at some point (but then I pushed another comment to fix some python style issue), so I think the integration part of the this is now good.

My main concern is the fact that the issue you're having could be solved in a different way by calling compile with the --export-binaries flag so that binaries are exported build/ and let clangd read it from there.
I wouldn't make this big of change in the CLI to solve an issue that only requires some more configuration. confused

Yeah, I also looked at a solution like that, but that requires explicitly passing the path the compilation_commands.json to clangd, which seems tricky at least in my environment (I'm using YouCompleteMe in vim, which I think also looks at compile_commands.json, but is not so configurable in this regard). I could look closer at that, though.

However, note that even when the last commit of this PR is left out (the compile_commands.json exception), I think the other changes made have merit, so I think it would still be worth discussing those (and as mentioned, maybe not the exact implementation, but the behavior, and the testcases are also already fine like this).

@silvanocerza
Copy link
Contributor

Heh. In any case, I think I did see the Windows integration tests pass at some point (but then I pushed another comment to fix some python style issue), so I think the integration part of the this is now good.

Should be fine now, I already rerunned the failing jobs.

Yeah, I also looked at a solution like that, but that requires explicitly passing the path the compilation_commands.json to clangd, which seems tricky at least in my environment (I'm using YouCompleteMe in vim, which I think also looks at compile_commands.json, but is not so configurable in this regard). I could look closer at that, though.

I think you can do it something like this:

let g:ycm_compilation_database_folder = 'build'

From here: ycm-core/YouCompleteMe#2741 (comment)

I didn't test it, I don't use YCM on Vim.

However, note that even when the last commit of this PR is left out (the compile_commands.json exception), I think the other changes made have merit, so I think it would still be worth discussing those (and as mentioned, maybe not the exact implementation, but the behavior, and the testcases are also already fine like this).

Yeah, the other changes are interesting but am still thinking if I like the changes in arduino/go-paths-helper#13, probably I'd use a different approach. 🤔

@matthijskooijman
Copy link
Collaborator Author

Seems I forgot to reply here, I was prompted by #1765 to have a look again here.

I think you can do it something like this:

Seems like that approach would indeed work for YouCompleteMe in vim, but does require a bunch of custom python code, so is very specific to YCM (i.e. it's not just a matter of passing some option to clangd) and unlikely to be easily supported in other environments too.

Regardless of the particulars of the compilation_commands.json usecase, the issue is maybe more generic: Under what circumstances should a broken symlink affect the build? It seems #1765 is essentially this same problem, but triggered by a symlink created by git (or the user, not entirely sure).

@matthijskooijman matthijskooijman mentioned this pull request Jun 14, 2022
Closed
3 tasks
@matthijskooijman
Copy link
Collaborator Author

I found out that clangd (tested with version 13) actually already seems to look inside build directories for compile_commands.json, even upwards in the directory hierarchy (e.g. for competion for MySketch/src/foo.cpp, it looks at MySketch/src/build/compile_commands.json, then MySketch/build/compile_commands.json, and then even further up the tree).

So that means that simply exporting compile_commands.json into the build directory when passing -e, as @silvanocerza suggested previously, would "just work" when using clangd, so that seems a good thing to implement, solving the compile_commands.json issue neatly.

Also, note that since version 11, clangd can also read .clangd config files (also upwards through the tree), which can be used to tweak the compilation options, or read compile_commands.json from non-standard locations. For this particular case it is not needed, but might be interesting for anyone reading this. See https://clangd.llvm.org/config

As said before, improving handling of (broken) symlinks is still relevant regardless of the comile_comands.json case.

@matthijskooijman
Copy link
Collaborator Author

So that means that simply exporting compile_commands.json into the build directory when passing -e, as @silvanocerza suggested previously, would "just work" when using clangd, so that seems a good thing to implement, solving the compile_commands.json issue neatly.

Hm, I just realized that the obvious way to implement this (and also what @silvanocerza suggested) would be to put the compile_commands.json file along with the other exported files into build/<FQBN>/, not directly into the build directory. However, clangd does not check that location.

This can be fixed by adding a .clangd config file in the sketch directory, to explicitly locate the compilation database, e.g.

$ cat .clangd
CompileFlags:
        CompilationDatabase: build/STMicroelectronics.stm32.Nucleo_64

Unfortunately that does not work out of the box, and is also not configurable globally (because the global .clangd config file cannot use relative paths, and also the board name might be different for different sketches/projects).

Maybe we could consider exporting the file to build/<FQBN>/ when -e is given, and maybe add an extra --export-compilation-database or so that would cause exporting the file (also) to build/? Or maybe always (also) export there (or make a symlink so build/compile_commands.json always points to the most recently exported file (which prevents the broken symlink problem this issue is about, since the build directory is not cleaned up on reboot)?

@per1234 per1234 added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Jun 27, 2022
@per1234 per1234 assigned cmaglie and unassigned silvanocerza Jun 27, 2022
@cmaglie cmaglie mentioned this pull request Jan 11, 2024
Merged
6 tasks
@matthijskooijman
Add test for working and broken symlinks
1e1db0a 
This tests that a symlink to an external file is compiled correctly, and
tests the current behavior of breaking on any broken symlink in the
sketch directory.
@matthijskooijman
Draft: Upgrade go-paths-helper to patched version
38f72ef 
This commit should not be merged, instead the indicated commit should be
merged into upstream go-paths-helper first and this commit adapted.

This makes two relevant changes:
 - ReadDirRecursive now returns broken symlinks as-is (and other files
   that cannot be stat'd due to permission errors) rather than failing
   entirely. This causes broken symlinks to no longer break the build,
   unless they are actually used (i.e. broken .cpp links will cause gcc
   to error out later).
 - FilterOutDirs no longer filters out broken symlinks (and other files
   that cannot be stat'd due to whatever error). This ensures that
   broken symlinks are actually returned by Sketch.supportedFiles, so
   sketch.New can check them (though that still ignores any errors
   currently).

The test suite is updated for these changes.
@matthijskooijman
Fail when sketch files cannot be opened during load
2f0db25 
Before commit e7d4eaa ([breaking] Refactor codebase to use a single
Sketch struct (arduino#1353)), any sketch file that could not be opened would
report a fatal error, but the behavior was changed to silently skip such
failing files instead.

This restores the original behavior and fails to load a sketch of some
of the contained files (after filtering on file extension, so this only
includes files that are actually intended to be processed), instead of
silently excluding the files (which likely produces hard to explain
compilation errors later).
@matthijskooijman
Draft: Exclude compile_commands.json from sketch loading
444c506 
This allows users to create a symlink to the autogenerated file in the
sketch directory, without that file being handled or causing failure
when the target file does not exist yet (or anymore).

Draft: This abuses FilterOutSuffix, it should match the filename, but
go-paths-helper does not support this now.

Draft: Is this really the right place/approach to exclude this file?
@matthijskooijman matthijskooijman force-pushed the skip-unused-broken-symlinks-v2 branch from 3c9951e to 444c506 Compare January 12, 2024 07:36
@matthijskooijman
Copy link
Collaborator Author

I've squashed the fixup commits, just in case this PR could still serve as inspiration (after seeing #2497 that touches on the same issue).

@cmaglie
Copy link
Member

cmaglie commented Jan 17, 2024

Superseded by #2497

@cmaglie cmaglie closed this Jan 17, 2024
@cmaglie cmaglie added the conclusion: declined Will not be worked on label Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@cmaglie cmaglie

Labels
conclusion: declined Will not be worked on topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@matthijskooijman @silvanocerza @cmaglie @per1234